Repository navigation
Fix: an onion path could run through the node it was addressed to - #2221
Merged
mpretty-cyro merged 1 commit intoSep 28, 2026
Merged
mpretty-cyro merged 1 commit into
mpretty-cyro merged 1 commit into
Conversation
The path build never told the path manager what the request was for, so a path could contain the destination and put the last hop and the target on the same node. A node will not connect to itself, which broke quic-to-quic connections on 2.12; the service nodes now detect the self-connection and work around it, so the symptom is masked server-side while the client still builds the path. getPath already takes a snode to exclude and selectPath already honours it - the mechanism was simply never used. Only a snode destination can collide, so a server destination excludes nothing. This is avoidance, not a guarantee: if every path contains the destination, selectPath logs and returns one anyway. Closing that needs a different exclusion mechanism and is deliberately not attempted here. Path overrides bypass getPath entirely, so they are unaffected. Today their only producer is the path test, which already picks a destination outside the path it is testing.
mpretty-cyro
marked this pull request as ready for review
September 28, 2026 01:55
Bilb
approved these changes
Sep 28, 2026
mpretty-cyro
added a commit
to mpretty-cyro/session-android
that referenced
this pull request
Sep 28, 2026
Follow-ups to the destination-exclusion change in session-foundation#2221, now that it is on dev. Snode equality is address and port, so the exclusion missed a node whose pool record and swarm record were fetched either side of an IP or port change - the same node under two addresses, one of which is the path's. Path selection now compares ed25519 keys, falling back to equality for a snode carrying no key material. Kept local to path selection rather than changing Snode.equals, which the pool, the paths and the swarm all rely on. Also drops the claim that quic-to-quic "refuses it outright" from the comment explaining the exclusion. That describes the service nodes' behaviour, which has since changed - they detect the self-connection and work around it - and nothing here can notice when it changes again. The local obligation is the durable half: a path must not contain the node it is addressed to.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The path build never told the path manager what the request was for, so a path could contain the destination and put the last hop and the target on the same node. A node will not open a connection to itself, which breaks quic-to-quic connections on 2.12.
The symptom is currently masked server-side: service nodes now detect the self-connection and de-onion the next hop in place. The client still builds the bad path, so this is worth fixing on its own terms rather than as a live breakage.
The change
getPathhas always taken a snode to exclude, andselectPathhas always honoured it — the mechanism was simply never used. The one real caller now passes what was already in scope:OnionDestinationis sealed and onlySnodeDestinationcan collide; aServerDestinationis not a pool member, so it excludes nothing.What this does not do
selectPathlogs "No valid paths excluding requested snode, using any available path" and returns one.getPathfirst falls through torebuildPaths(reusablePaths = current), which reuses the existing paths, so a rebuild need not produce a non-colliding one. Rare with 2 paths of 3 hops against a pool of hundreds. Closing it would mean a broader exclusion mechanism, deliberately not attempted here.getPathentirely. Their only producer today is the path test, which already picks a destination outside the path it is testing — a property of that caller, not something enforced where the override is consumed.Tests
Three in
OnionSessionApiExecutorTest, following that file's existing mockk harness. Two of them are discriminators, verified red with the production change reverted:onionBuilder.build, given a colliding path and a clean one to choose betweenThe third passes either way: before the change the call passes the default
null, and so does the fixed code for a server destination. It guards against a futureexcludethat is wrong for the server case; it is not evidence for this one.Unit suite: 306 pass, 0 fail (303 on this base plus these three).